So it illustrate how to fix data race. We have to integrate your code in the default font cache.

For library users, the two globals to think about are FontCache and GlyphCache.

FontCache already offers library users the option of supplying an interface implementation that synchronizes map access. That's what I did in the example. In the PR testFontCache implements the interface adding mutex protection. (So I add protection and accept the performance penalty it entails.)

What isn't in the library is an equivalent mechanism for GlyphCache - so the test case is intended as an illustration for the GlyphCache problem.

I'm not sure that's the end of it, though: as an experiment I disabled the glyph cache altogether and still ran into race conditions.

I should add that it's entirely possible my TestFontCache as given in the PR is faulty. I'd be tempted to make both caches properties of the context struct, but that's too far from the current approach.

I believe @redstarcoder is looking into a mechanism that allows the user to protect GlyphCache in the way FontCache can already be protected. I've started to try something similar, but I'm sure @redstarcoder's version will be much better as I'm new to the codebase.

thanks @gerald1248,
for a first implementation it's acceptable I think
another way to fix the issue is to remove the globals and be able to configure with a FontCache and a GlyphCache that could be global and synchronized or not.

@llgcode, @redstarcoder: I've simplified the test case so it just calls the library and doesn't attempt any trickery with either the font cache or the glyph cache.

Having spent some time mulling this over I really think that a potential race condition when calling a 2D drawing library from a server handler is a bug.

On balance I think the default needs to be threadsafe. Setters like SetFontCache and SetGlyphCache are great, but they should be the tools to squeeze out the last bit of performance when concurrency isn't a requirement.

In other words: the library really needs to pass go test -race -test.v sync_test.go.

Currently the library isn't thread-safe by default and hasn't ever been, this makes me think that most users expect this behaviour at this point. I also don't see it as a huge issue as I always check if my libraries are thread-safe or not by default anyways when I'm dealing with multi-threaded applications (and I think others usually do the same...). This isn't to say that things can't change, as we do want to refactor areas of code for a future release, I just don't think this is an easy one to justify.

If we did make it thread-safe by default we'd have to have new caches generated with each GraphicContext, mutex lock thread unsafe operations, or utilise channels someway (which would probably lead to some restructuring...).

If we have new caches generated with each GraphicContext we would murder performance in text drawing routines. You can see when the GlyphCache was implemented in #118, we'd lose that 50% speed increase for many applications.

Mutex locks are another option, but they're hackish to me as a default here. Not only will they reduce performance in single-threaded applications, we don't have any data by how much, or how much of an increase in speed a mutex locked multi-threaded application observes. Ideally for 4 threads you'd have 4 caches, problem solved (but I don't see any obvious way the library could do that for you...).

Using channels is hopefully something we never do, but it's there as an option. With the current model, it'd be slower than mutex locks (they work like fancy mutex locks).

I disagree that the library should be thread-safe by default. Unless you can propose a solution that doesn't result in a loss of performance in single-threaded applications (what seems to be the most common use), or at least one that results in a model we can all be happy with. As it stands new caches with each GC hurts both, mutex locks hurt single-threaded (and aren't ideal for multi-threaded), and channels are pretty much out of the scope completely.

I think we should make it clear in the header of the documentation that draw2d isn't thread-safe by default. That way people at least won't expect it to be thread-safe. Maybe we could even throw in an example of a multi-threaded application too so newer programmers won't have to come up with their own code.

hi @redstarcoder, thanks for taking the time to reply in detail. I can see where you're coming from. I'm not suggesting not having a glyph cache or a font cache; they're both great optimizations.

For my use case fussing over thread safety is overkill - I only have one context, use it and then return the base64 encoded string of the image in the request response. (Not loading fonts from disk is really important to me and something the font cache enables for me.)

The PR wasn't working as it was, so I made it a simple test that only passes if default thread safety is a goal. If it isn't, I do understand and that's fine too.

Hi @redstarcoder @gerald1248 ,
happy new year :-)
draw2d have to be globally thread safe, we must be able to instanciate a gc without woorying about thread safety, (because a lot of usage is to be used on a server). A Gc itself can't be thread safe, it can't be shared between routines. We use caches for improving performance so we need the best of both world. I don't like mutex, moreover in a drawing library. So the best way to go forward is to remove global map/objects that are not thread safety.

Happy New Year! @llgcode alright well I'm even more glad I wrote out a breakdown of options then! In PR #128 I can implement a local cache and abolish the global default. Then I think we just have to do another PR for localising defaultFonts and fontCache in font.go. Any other similar globals we need to worry about?

@gerald1248 looks like you're getting exactly what you wanted anyways :). This is now clearly in the scope of the project. Have you noticed any other globals we need to worry about?

@redstarcoder I haven't but once the font and glyph caches are local to the context, go test -race -test.v sync_test.go should tell us if there's anything else to make threadsafe :)

Yes we can create another PR to remove global font cache to understand the impact and communicate on this if needed. I don't see other globals.
Thanks very much for your help.